Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boards: add nimble defconfig to esp32c3-generic #15187

Merged
merged 2 commits into from
Dec 25, 2024

Conversation

Laczen
Copy link
Contributor

@Laczen Laczen commented Dec 13, 2024

Note: Please adhere to Contributing Guidelines.

Summary

Provide nimble configuration for esp32c3.

Impact

Testing

Verified on wsl2: ubuntu 22.04

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Board: risc-v Size: S The size of the change in this PR is small labels Dec 13, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 13, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change, it lacks crucial information. Here's a breakdown of what's missing:

  • Summary: While it states what is being done (providing nimble config), it doesn't explain why (is it a new feature, bug fix, etc.), how it works (details of the nimble configuration), or reference related issues.
  • Impact: This section is completely empty. It needs to address all the listed points (impact on user, build, hardware, documentation, security, compatibility). Even if the answer is "NO" for some, it should be explicitly stated.
  • Testing: While the host OS is mentioned, compiler information is missing. Crucially, the target information is incomplete. While esp32c3 is mentioned, the specific board and configuration are not. And most importantly, there are no "before" and "after" testing logs. These logs are essential to demonstrate the change's effect and that it works as intended.

To make this PR compliant, you need to add the missing information in each section. For example:

Example of a more complete Summary:

This change adds nimble configuration for the esp32c3 to enable Bluetooth Low Energy (BLE) functionality. It adds a new Kconfig file for nimble under the esp32c3 architecture and updates the board configuration to enable it. This addresses NuttX Issue #XXX which requested BLE support for esp32c3.

Example of a more complete Impact:

  • Is new feature added? YES (Adds BLE support via nimble)
  • Impact on user (will user need to adapt to change)? YES (Users will need to enable nimble in their configuration and may need to install additional tools for BLE development)
  • Impact on build (will build process change)? YES (Nimble will be included in the build if enabled)
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? YES (Affects esp32c3 architecture and boards using it)
  • Impact on documentation (is update required / provided)? YES (Documentation will need to be updated to describe how to configure and use nimble on esp32c3)
  • Impact on security (any sort of implications)? NO
  • Impact on compatibility (backward/forward/interoperability)? NO
  • Anything else to consider? None

Example of more complete Testing:

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): OS (Ubuntu 22.04 on WSL2), CPU(x86_64), compiler(GCC 11.3.0)
  • Target(s): arch(esp32c3), board:esp32c3-devkit:default

Testing logs before change:

 (Log output showing the lack of BLE functionality, e.g., attempting a BLE scan and failing)

Testing logs after change:

(Log output showing successful BLE operation, e.g., successful scan results)

By filling in the missing information like this, your PR will better meet the requirements and be easier for reviewers to assess.

@acassis
Copy link
Contributor

acassis commented Dec 13, 2024

@Laczen please take a look at this issue:

Error: /github/workspace/sources/apps/wireless/bluetooth/nimble/mynewt-nimble/nimble/host/src/ble_gap.c:6979:1: error: 'ble_gap_event_listener_call' defined but not used [-Werror=unused-function]
 6979 | ble_gap_event_listener_call(struct ble_gap_event *event)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

@Laczen
Copy link
Contributor Author

Laczen commented Dec 14, 2024

@Laczen please take a look at this issue:

Error: /github/workspace/sources/apps/wireless/bluetooth/nimble/mynewt-nimble/nimble/host/src/ble_gap.c:6979:1: error: 'ble_gap_event_listener_call' defined but not used [-Werror=unused-function]
 6979 | ble_gap_event_listener_call(struct ble_gap_event *event)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

This is a bit annoying, the correction of this error (if at all possible) is done in a different repo (nuttx-apps).

@raiden00pl
Copy link
Member

@Laczen I see that you don't configure any BLE role for nimble. Probably this is why this warning happens, and probably this is why you need ifup bnep0. You should add CONFIG_NIMBLE_ROLE_PERIPHERAL=y to configuration.

Add a configuration for ble using nimble on esp32c3-generic

Signed-off-by: Laczen JMS <[email protected]>
Modify documentation of esp32c3 and add nimble section to
esp32c3-generic.

Signed-off-by: Laczen JMS <[email protected]>
@Laczen
Copy link
Contributor Author

Laczen commented Dec 15, 2024

@acassis, it seems the PR is still failing CI. How can I see the error, I've tried looking at the build failure output, but the failure is unclear for me.

@acassis
Copy link
Contributor

acassis commented Dec 15, 2024

@Laczen the issue seems related to other board config:

	boards/risc-v/qemu-rv/rv-virt/configs/citest/logs/

You can try to update your upstream, rebase your branch against it and send a git push -f

@lupyuen please take a look

@lupyuen
Copy link
Member

lupyuen commented Dec 16, 2024

@Laczen The error is caused by something else:
https://github.com/apache/nuttx/actions/runs/12329209626/job/34414942581?pr=15187#step:7:320

Configuration/Tool: rv-virt/citest
test_open_posix/test_openposix_.py::test_ltp_interfaces_pthread_barrierattr_init_2_1 FAILED [ 17%]

Which we're still fixing (sorry I'm a bit overloaded sigh):

FYI If we're not sure if our PR is causing the CI Build to fail, we might check nuttx-dashboard.org. This shows that rv-virt/citest is indeed broken, so it's not because of our PR :-)

Screenshot 2024-12-16 at 11 53 08 AM

@Laczen
Copy link
Contributor Author

Laczen commented Dec 24, 2024

@acassis should I rebase?

@lupyuen
Copy link
Member

lupyuen commented Dec 24, 2024

@Laczen There's a problem with CITest, I'm patching it here:

@Laczen
Copy link
Contributor Author

Laczen commented Dec 24, 2024

@Laczen There's a problem with CITest, I'm patching it here:

Ok, if I need to rebase just let me know.

@lupyuen
Copy link
Member

lupyuen commented Dec 25, 2024

Sorry @Laczen could you rebase with master? It should fix the SCANFTEST error. Thanks :-)
https://github.com/apache/nuttx/actions/runs/12329209626/job/34848165076?pr=15187#step:7:1267

@xiaoxiang781216 xiaoxiang781216 merged commit 28937fe into apache:master Dec 25, 2024
15 of 18 checks passed
@Laczen Laczen deleted the esp32c3_nimble branch December 25, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Board: risc-v Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants